Skip to content

#185: Added subClassOfInShapesGraph parameter #447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Aug 5, 2025
Merged

#185: Added subClassOfInShapesGraph parameter #447

merged 7 commits into from
Aug 5, 2025

Conversation

HolgerKnublauch
Copy link
Contributor

Closes #185

Added Andy and Ashley as reviewers too, to check if this is implementable from their engines point of view. The main difference is that instead of

?instance rdf:type/rdfs:subClassOf $class .

it would become

?instance rdf:type ?type .
GRAPH $shapesGraph {
    $type rdfs:subClassOf* ?class
}

And it's an optional feature...

jeswr
jeswr reviewed Jul 29, 2025
View reviewed changes
@jeswr
Copy link
Member

jeswr commented Jul 29, 2025 •
edited
Loading

At risk of overcomplicating things - would it be worth inferenceDataInShapesGraph rather than subClassOfInShapesGraph; and using this feature to enable inclusion of other data for inference rules in the shapes graph?

@HolgerKnublauch
Copy link
Contributor Author

@jeswr there is already sh:entailment to activate other inferencing. And it is under complete control of the user which graph to pass into the engine as shapes graph. That graph may have been pre-processed with OWL to infer additional rdfs:subClassOf axioms.

As the above IMHO doesn't need a formal parameter to the engine, I wanted to however spell out the new flag because it does need a parameter as it changes how some terms (SHACL Instance) behave.

@afs
Copy link
Contributor

afs commented Jul 29, 2025

SHACL implementations can be parameterized

Where's the list of all parameters?

The whole "implementation controls" area seems to sit uneasily between implementation and standardization.

@afs afs mentioned this pull request Jul 29, 2025
Closed
@afs
Copy link
Contributor

afs commented Jul 29, 2025

Not sure if the discussion is best here or on #185.

Added a general comment on the issue -- #185 (comment)

@HolgerKnublauch
Copy link
Contributor Author

I am not aware of other flags or parameters for the engine, but they could be grouped into an appendix if we define more.

YoucTagh
YoucTagh approved these changes Aug 4, 2025
View reviewed changes
Copy link
Contributor

@YoucTagh YoucTagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am approving this pull request (PR) because I favour using instead of over in addition to, as the latter would add more complexity. Also, if we gather other flags or parameters, we can add them in an appendix in another PR.

@afs
Copy link
Contributor

afs commented Aug 4, 2025

No test?

@HolgerKnublauch
Copy link
Contributor Author

@afs correct, no test because the feature is optional ("SHOULD") and we don't have any mechanism in the test case framework to set this flag.

ajnelson-nist
ajnelson-nist reviewed Aug 4, 2025
View reviewed changes
TallTed
TallTed requested changes Aug 4, 2025
View reviewed changes
@TallTed
Copy link
Member

TallTed commented Aug 4, 2025

@HolgerKnublauch — SHOULD statements are as deserving of tests as MUST statements. Failing a SHOULD test should not result in failure of conformance report, but should result in reporting that failure of SHOULD such that deployers can differentiate between two implementations that both satisfy all MUST statements but satisfy different (whether counts or instances) SHOULD statements.

HolgerKnublauch and others added 3 commits August 4, 2025 16:50
@HolgerKnublauch @TallTed
Apply suggestions from code review
93a7467 
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
@HolgerKnublauch @jeswr
Update shacl12-core/index.html
d6e6989 
Co-authored-by: Jesse Wright <63333554+jeswr@users.noreply.github.com>
@HolgerKnublauch
Updated change entry to new contract
330abac
@HolgerKnublauch HolgerKnublauch requested a review from TallTed August 4, 2025 14:56
@HolgerKnublauch
Copy link
Contributor Author

Ok, the semantics of the flag have been changed to be the union of shapes and data graphs.

Please re-check and let me know any remaining concerns.

ajnelson-nist
ajnelson-nist reviewed Aug 4, 2025
View reviewed changes
ajnelson-nist
ajnelson-nist reviewed Aug 4, 2025
View reviewed changes
@ajnelson-nist ajnelson-nist mentioned this pull request Aug 4, 2025
Open
ajnelson-nist
ajnelson-nist reviewed Aug 4, 2025
View reviewed changes
shacl12-core/index.html
Comment on lines +2743 to +2745
SHACL processors SHOULD offer a parameter <code>subClassOfInShapesGraph</code> that, if set to <code>true</code>,
should alter the definition of <a>SHACL Type</a> so that the <code>rdfs:subClassOf</code> triples are queried
from the <a>shapes graph</a> in addition to the <a>data graph</a>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking remark, anyone feel free to Resolve this thread: #431 may open an avenue for writing tests for this.

ajnelson-nist
ajnelson-nist reviewed Aug 4, 2025
View reviewed changes
ajnelson-nist
ajnelson-nist reviewed Aug 4, 2025
View reviewed changes
Copy link
Contributor

@ajnelson-nist ajnelson-nist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overall looks good to me. I left a few minor suggestions, but didn't see anything meriting blocking with the "Request changes" review.

I think I'm now understanding @afs 's question on parameters. We have subClassOfInShapesGraph in this PR, and potentially one representing conformance-disallowance depending on how #453 shakes out.

Definitely if we have a second parameter to accompany subClassOfInShapesGraph, a table, section, or appendix should go in for indexing specification-defined parameters.

I could probably be swayed to wait to sketch that table/section, but request if the decision is to wait that an Issue be posted to track it.

@afs
Copy link
Contributor

afs commented Aug 4, 2025

@afs correct, no test because the feature is optional ("SHOULD") and we don't have any mechanism in the test case framework to set this flag.

Tests are more than just compliance. They also perform the role of another explanation of the feature. For a large system such as SHACL, tests are valuable implementation resources.

e.g. where would constraints be affected when this optional feature is activated?

HolgerKnublauch and others added 2 commits August 4, 2025 21:19
@HolgerKnublauch @ajnelson-nist
Update shacl12-core/index.html
385a83d 
Co-authored-by: Alex Nelson <alexander.nelson@nist.gov>
@HolgerKnublauch @ajnelson-nist
Update shacl12-core/index.html
47ab3a7 
Co-authored-by: Alex Nelson <alexander.nelson@nist.gov>
@HolgerKnublauch HolgerKnublauch merged commit fea6669 into gh-pages Aug 5, 2025
1 check passed
@HolgerKnublauch HolgerKnublauch deleted the issue-185 branch August 5, 2025 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajnelson-nist ajnelson-nist ajnelson-nist left review comments

@bergos bergos bergos approved these changes

@TallTed TallTed TallTed approved these changes

@YoucTagh YoucTagh YoucTagh approved these changes

@jeswr jeswr jeswr approved these changes

@afs afs Awaiting requested review from afs

@ashleysommer ashleysommer Awaiting requested review from ashleysommer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Where should my ontology go? Data graph versus shapes graph
7 participants
@HolgerKnublauch @jeswr @afs @TallTed @bergos @ajnelson-nist @YoucTagh